rename sandbox fields#16
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR systematically renames resource configuration and API field names across the codebase: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
leap0/_async/sandbox.py (1)
229-268: Consider aligning asynccreate()telemetry flags with sync API parity.
SandboxesClient.create(sync) supports bothotel_exportand legacytelemetry, while async currently accepts onlyotel_export. Keeping parity reduces migration friction and surprises across clients.Parity-oriented refactor (optional)
async def create( self, *, template_name: str = DEFAULT_TEMPLATE_NAME, vcpu: int = DEFAULT_VCPU, memory: int = DEFAULT_MEMORY_MIB, timeout: int = DEFAULT_TIMEOUT, auto_pause: bool = False, - otel_export: bool = False, + otel_export: bool | None = None, + telemetry: bool | None = None, env_vars: dict[str, str] | None = None, network_policy: NetworkPolicyDict | None = None, http_timeout: float | None = None, ) -> AsyncSandboxT | SandboxData | SandboxStatus: @@ - params = CreateSandboxParams( + effective_otel_export = otel_export if otel_export is not None else bool(telemetry) + params = CreateSandboxParams( template_name=template_name, vcpu=vcpu, memory=memory, timeout=timeout, auto_pause=auto_pause, - otel_export=otel_export, - env_vars=_inject_otel_env(env_vars) if otel_export else env_vars, + otel_export=effective_otel_export, + env_vars=_inject_otel_env(env_vars) if effective_otel_export else env_vars, network_policy=network_policy, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@leap0/_async/sandbox.py` around lines 229 - 268, The async create() method lacks the legacy telemetry flag present in the sync SandboxesClient.create, causing API parity issues; add an optional telemetry: bool | None parameter to the async method signature, thread it through to CreateSandboxParams (alongside otel_export) and ensure _inject_otel_env is invoked when either telemetry or otel_export indicates telemetry should be enabled, keeping behavior consistent with the sync implementation and preserving the otel_export-specific env handling.tests/_async/test_sandboxes.py (1)
19-23: Strengthen create-request assertions for renamed fields.This test now exercises the new API shape, but it only asserts
template_name. Please also assert the outbound payload usesmemory/timeoutand does not include legacy keys, so future regressions are caught early.Proposed test assertion update
- result = await AsyncSandboxesClient(async_mock_transport, sandbox_domain="s.dev").create(template_name="my-tpl", vcpu=2, memory=2048) + result = await AsyncSandboxesClient(async_mock_transport, sandbox_domain="s.dev").create( + template_name="my-tpl", + vcpu=2, + memory=2048, + timeout=300, + ) args, kwargs = async_mock_transport.request_json.call_args assert args == ("POST", "/v1/sandbox") assert kwargs["json"]["template_name"] == "my-tpl" + assert kwargs["json"]["memory"] == 2048 + assert kwargs["json"]["timeout"] == 300 + assert "memory_mib" not in kwargs["json"] + assert "timeout_min" not in kwargs["json"] assert result.id == "sbx-1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_async/test_sandboxes.py` around lines 19 - 23, The test should also assert the full outbound JSON payload uses the new field names and omits legacy keys: after calling AsyncSandboxesClient(...).create(...), inspect async_mock_transport.request_json.call_args and assert kwargs["json"]["vcpu"] == 2 and kwargs["json"]["memory"] == 2048, assert there is a "timeout" key present (use the default or configured value expected by create), and assert that legacy keys such as "size" and "template" are not present in kwargs["json"] so regressions are caught early.tests/_sync/test_sandboxes.py (1)
18-22: Add explicit assertions for renamed request keys in sync create test.Line 18 now exercises the renamed params, but the assertion block doesn’t verify emitted key names. Assert
memory/timeoutpresence and legacy-key absence to lock this contract.Proposed assertion hardening
- result = SandboxesClient(mock_transport, sandbox_domain="s.dev").create(template_name="my-tpl", vcpu=2, memory=2048) + result = SandboxesClient(mock_transport, sandbox_domain="s.dev").create( + template_name="my-tpl", + vcpu=2, + memory=2048, + timeout=300, + ) args, kwargs = mock_transport.request_json.call_args assert args == ("POST", "/v1/sandbox") assert kwargs["json"]["template_name"] == "my-tpl" + assert kwargs["json"]["memory"] == 2048 + assert kwargs["json"]["timeout"] == 300 + assert "memory_mib" not in kwargs["json"] + assert "timeout_min" not in kwargs["json"] assert result.id == "sbx-1"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/_sync/test_sandboxes.py` around lines 18 - 22, The test for SandboxesClient.create is missing assertions that verify the renamed request keys and the removal of legacy keys; after calling SandboxesClient(...).create(...) inspect mock_transport.request_json.call_args and add assertions that kwargs["json"] contains "memory" and "timeout" keys and that the legacy keys (e.g., "memory_mb" and "timeout_seconds") are not present, referencing the call via mock_transport.request_json.call_args and the SandboxesClient.create invocation to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@leap0/models/sandbox.py`:
- Around line 194-196: The code is silently defaulting missing/renamed sandbox
fields to 0 (memory/disk/timeout), so add a strict helper like
_require_int(data, key, legacy_key=None) that first checks data[key], falls back
to legacy_key if provided, raises ValueError if neither is present, and converts
to int catching TypeError/ValueError to raise a clear error; replace the three
int(data.get(...,0)) calls for "memory", "disk", and "timeout" with calls to
_require_int (passing legacy keys if the API had previous names) and apply the
same replacement to the similar parsing at the other occurrence noted (the block
at the other three fields around lines 227-229).
---
Nitpick comments:
In `@leap0/_async/sandbox.py`:
- Around line 229-268: The async create() method lacks the legacy telemetry flag
present in the sync SandboxesClient.create, causing API parity issues; add an
optional telemetry: bool | None parameter to the async method signature, thread
it through to CreateSandboxParams (alongside otel_export) and ensure
_inject_otel_env is invoked when either telemetry or otel_export indicates
telemetry should be enabled, keeping behavior consistent with the sync
implementation and preserving the otel_export-specific env handling.
In `@tests/_async/test_sandboxes.py`:
- Around line 19-23: The test should also assert the full outbound JSON payload
uses the new field names and omits legacy keys: after calling
AsyncSandboxesClient(...).create(...), inspect
async_mock_transport.request_json.call_args and assert kwargs["json"]["vcpu"] ==
2 and kwargs["json"]["memory"] == 2048, assert there is a "timeout" key present
(use the default or configured value expected by create), and assert that legacy
keys such as "size" and "template" are not present in kwargs["json"] so
regressions are caught early.
In `@tests/_sync/test_sandboxes.py`:
- Around line 18-22: The test for SandboxesClient.create is missing assertions
that verify the renamed request keys and the removal of legacy keys; after
calling SandboxesClient(...).create(...) inspect
mock_transport.request_json.call_args and add assertions that kwargs["json"]
contains "memory" and "timeout" keys and that the legacy keys (e.g., "memory_mb"
and "timeout_seconds") are not present, referencing the call via
mock_transport.request_json.call_args and the SandboxesClient.create invocation
to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74f68575-3401-4bd1-ad27-e366bd82c949
📒 Files selected for processing (17)
leap0/_async/client.pyleap0/_async/sandbox.pyleap0/_async/snapshots.pyleap0/_schemas/sandbox.pyleap0/_schemas/snapshot.pyleap0/_sync/client.pyleap0/_sync/sandbox.pyleap0/_sync/snapshots.pyleap0/models/config.pyleap0/models/sandbox.pyleap0/models/snapshot.pytests/_async/test_sandboxes.pytests/_async/test_snapshots.pytests/_sync/test_sandboxes.pytests/_sync/test_snapshots.pytests/models/test_sandbox.pytests/models/test_snapshot.py
Comment on lines
+113
to
+114
| memory: int = DEFAULT_MEMORY_MIB | ||
| timeout: int = DEFAULT_TIMEOUT |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider a short compatibility window for the renamed public fields.
These names changed on public request/response models in-place, so downstream code still using memory_mib, disk_mib, or timeout_min will break on upgrade. If this is not meant to be a hard breaking release, keep deprecated aliases/properties for one cycle and serialize only the new wire names.
Also applies to: 175-177, 209-211
Comment on lines
+194
to
+196
| memory=int(data.get("memory", 0)), | ||
| disk=int(data.get("disk", 0)), | ||
| timeout=int(data.get("timeout", 0)), |
There was a problem hiding this comment.
Don't silently turn missing renamed fields into 0.
If the backend omits one of these keys—or still returns the legacy names during rollout—both parsers manufacture 0 values instead of surfacing the contract mismatch. That makes a mixed-version response look valid and can leak bogus resource limits to callers. Prefer failing fast here, or temporarily falling back to the legacy keys while the API rolls out.
💡 Minimal hard-fail / fallback shape
- memory=int(data.get("memory", 0)),
- disk=int(data.get("disk", 0)),
- timeout=int(data.get("timeout", 0)),
+ memory=_require_int(data, "memory", legacy_key="memory_mib"),
+ disk=_require_int(data, "disk", legacy_key="disk_mib"),
+ timeout=_require_int(data, "timeout", legacy_key="timeout_min"),def _require_int(data: Mapping[str, object], key: str, *, legacy_key: str | None = None) -> int:
value = data.get(key)
if value is None and legacy_key is not None:
value = data.get(legacy_key)
if value is None:
raise ValueError(f"sandbox response missing required field {key!r}")
try:
return int(value)
except (TypeError, ValueError) as err:
raise ValueError(f"sandbox response has invalid {key!r}: {value!r}") from errAlso applies to: 227-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@leap0/models/sandbox.py` around lines 194 - 196, The code is silently
defaulting missing/renamed sandbox fields to 0 (memory/disk/timeout), so add a
strict helper like _require_int(data, key, legacy_key=None) that first checks
data[key], falls back to legacy_key if provided, raises ValueError if neither is
present, and converts to int catching TypeError/ValueError to raise a clear
error; replace the three int(data.get(...,0)) calls for "memory", "disk", and
"timeout" with calls to _require_int (passing legacy keys if the API had
previous names) and apply the same replacement to the similar parsing at the
other occurrence noted (the block at the other three fields around lines
227-229).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
memory_mib/disk_mibandtimeout_mintomemory/diskand second-basedtimeoutSummary by CodeRabbit
memory_mib→memory,disk_mib→disktimeout_min→timeout(now expressed in seconds rather than minutes)